fix(vcs): Fix base_ref / base_sha for non-github actions by chromy · Pull Request #2888 · getsentry/sentry-cli

chromy · 2025-10-29T13:33:42Z

This resolves EME-473

Both find_base_sha and git_repo_base_ref were not correct for non-Github workflow contexts.

find_base_ref returned previously returned a SHA where the intention of find_base_ref
was to return the nice name of the default merge branch of the remote. So in the common case:

---o---U---o---o---o---H

Where:

H is some-branch and
U is origin/main and
refs/remotes/origin/HEAD is refs/remotes/origin/main

git_repo_base_ref should return main

find_base_sha did not handle the not Github case when it should return the equivalent of:
git merge-base HEAD origin/HEAD. For find_base_sha we want the merge base of
of U and H. In the above example this is the SHA of U. However they can be different:
e.g.

    o---o---o---origin/main
   /
---1---o---o---o---foo

find_base_ref should still return main
but find_base_sha should be the SHA of 1.

linear · 2025-10-29T13:34:12Z

EME-473 `base_branch` picking up `base_sha`, `base_sha` picking up nothing (sentry-cli)

src/utils/vcs.rs

runningcode

nice find and fix!!

szokeasaurusrex

Hey @chromy, I left a couple comments, but I am finding it difficult to review this change because it appears this PR is combining a bug fix (fixing git_repo_base_ref) and a new feature (adding support for non-GitHub environments to find_base_sha, which previously, was clearly only designed to handle GitHub environments), plus a minor refactor (folding find_merge_base_ref into git_repo_base_ref), all into one.

I would appreciate if you could split this change into two PRs (one for the bug fix, one for the new feature), or possibly three (by splitting the find_merge_base_ref refactor further into its own PR) so I can have a better idea of which changes go together 🙏

I am also confused because the PR description is mentioning a function named find_base_ref, which does not exist. I assume you meant git_repo_base_ref, but would appreciate if you can confirm this and update the PR description accordingly when making the split PRs.

szokeasaurusrex · 2025-10-30T08:26:25Z

src/utils/vcs.rs

+        .resolve()?
+        .shorthand()
+        .ok_or(anyhow::anyhow!(
+            "Could not find remote tracking branch for {remote_name}"


shorthand() returns None if the shorthand is not valid UTF-8.

Suggested change

"Could not find remote tracking branch for {remote_name}"

"Remote branch name is not valid UTF-8"

Thanks! I've fixed this in the split patches.

szokeasaurusrex · 2025-10-30T08:32:32Z

src/utils/vcs.rs

 /// Extracts the PR base SHA from GitHub Actions event payload JSON.
-/// Returns Ok(None) if not a PR event or if SHA cannot be extracted.
-/// Returns an error if we cannot parse the JSON.
-fn extract_pr_base_sha_from_event(json_content: &str) -> Result<Option<String>> {
-    let v: Value = serde_json::from_str(json_content)
-        .context("Failed to parse GitHub event payload as JSON")?;
+/// Returns None if not a PR event or if SHA cannot be extracted.
+fn extract_pr_base_sha_from_event(json_content: &str) -> Option<String> {
+    let v: Value = match serde_json::from_str(json_content) {
+        Ok(v) => v,
+        Err(_) => {
+            debug!("Failed to parse GitHub event payload as JSON");
+            return None;
+        }
+    };

-    Ok(v.pointer("/pull_request/base/sha")
+    v.pointer("/pull_request/base/sha")
        .and_then(|s| s.as_str())
-        .map(|s| s.to_owned()))
+        .map(|s| s.to_owned())
 }


I see no reason for this function to change. If we want to debug-log the error message rather than error, the calling code should change how it handles the error returned from this function.

Suggested change

/// Extracts the PR base SHA from GitHub Actions event payload JSON.

/// Returns Ok(None) if not a PR event or if SHA cannot be extracted.

/// Returns an error if we cannot parse the JSON.

fn extract_pr_base_sha_from_event(json_content: &str) -> Result<Option<String>> {

let v: Value = serde_json::from_str(json_content)

.context("Failed to parse GitHub event payload as JSON")?;

/// Returns None if not a PR event or if SHA cannot be extracted.

fn extract_pr_base_sha_from_event(json_content: &str) -> Option<String> {

let v: Value = match serde_json::from_str(json_content) {

Ok(v) => v,

Err(_) => {

debug!("Failed to parse GitHub event payload as JSON");

return None;

}

};

Ok(v.pointer("/pull_request/base/sha")

v.pointer("/pull_request/base/sha")

.and_then(|s| s.as_str())

.map(|s| s.to_owned()))

.map(|s| s.to_owned())

}

/// Extracts the PR base SHA from GitHub Actions event payload JSON.

/// Returns Ok(None) if not a PR event or if SHA cannot be extracted.

/// Returns an error if we cannot parse the JSON.

fn extract_pr_base_sha_from_event(json_content: &str) -> Result<Option<String>> {

let v: Value = serde_json::from_str(json_content)

.context("Failed to parse GitHub event payload as JSON")?;

Ok(v.pointer("/pull_request/base/sha")

.and_then(|s| s.as_str())

.map(|s| s.to_owned()))

}

I disagree, there are two functions:

extract_pr_head_sha_from_event

extract_pr_base_sha_from_event

They ought to match exactly except for which SHA they extract.
Previously they didn't have the same implementation or API Result<Option<String>> vs. Option<String>. It does not make sense for one to handle invalid json and not the other.

Since invalid json from the Github API is (hopefully) rare I don't think it makes sense to push handling the case on to callers (making all calling code more complicated)*. Better options would be:

crashing

returning None and logging

I would generally lean towards crashing but since extract_pr_head_sha_from_event already had that behavior I matched that.

*There is actually only one real caller so I was tempted to inline these and simplify the code (c.f. http://number-none.com/blow/john_carmack_on_inlined_code.html) but since it has some nice tests I left it.

I have split this into it's own PR here: #2893

chromy · 2025-10-31T10:19:30Z

Hey @chromy, I left a couple comments, but I am finding it difficult to review this change because it appears this PR is combining a bug fix (fixing git_repo_base_ref) and a new feature (adding support for non-GitHub environments to find_base_sha, which previously, was clearly only designed to handle GitHub environments), plus a minor refactor (folding find_merge_base_ref into git_repo_base_ref), all into one.

I would appreciate if you could split this change into two PRs (one for the bug fix, one for the new feature), or possibly three (by splitting the find_merge_base_ref refactor further into its own PR) so I can have a better idea of which changes go together 🙏

I am also confused because the PR description is mentioning a function named find_base_ref, which does not exist. I assume you meant git_repo_base_ref, but would appreciate if you can confirm this and update the PR description accordingly when making the split PRs.

Okay!

I've split them into three:

chromy · 2025-10-31T10:19:40Z

(closing since split up now)

chromy requested review from a team and szokeasaurusrex as code owners October 29, 2025 13:33

This comment was marked as outdated.

Sign in to view

sentry bot reviewed Oct 29, 2025

View reviewed changes

src/utils/vcs.rs Outdated Show resolved Hide resolved

chromy force-pushed the chromy/2025-09-29-fix-base branch 6 times, most recently from 8ca51a4 to a5447c9 Compare October 29, 2025 16:09

This comment was marked as outdated.

Sign in to view

fix(vcs): Fix base_ref / base_sha for non-github actions

905715d

chromy force-pushed the chromy/2025-09-29-fix-base branch from a5447c9 to 905715d Compare October 29, 2025 16:14

runningcode approved these changes Oct 29, 2025

View reviewed changes

szokeasaurusrex reviewed Oct 30, 2025

View reviewed changes

chromy marked this pull request as draft October 30, 2025 16:49

chromy closed this Oct 31, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(vcs): Fix base_ref / base_sha for non-github actions#2888

fix(vcs): Fix base_ref / base_sha for non-github actions#2888
chromy wants to merge 1 commit intomasterfrom
chromy/2025-09-29-fix-base

chromy commented Oct 29, 2025 •

edited

Loading

Uh oh!

linear bot commented Oct 29, 2025

Uh oh!

This comment was marked as outdated.

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

runningcode left a comment

Uh oh!

szokeasaurusrex left a comment •

edited

Loading

Uh oh!

szokeasaurusrex Oct 30, 2025

Uh oh!

chromy Oct 31, 2025

Uh oh!

szokeasaurusrex Oct 30, 2025

Uh oh!

chromy Oct 30, 2025

Uh oh!

chromy commented Oct 31, 2025

Uh oh!

chromy commented Oct 31, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	"Could not find remote tracking branch for {remote_name}"
	"Remote branch name is not valid UTF-8"

Uh oh!

Conversation

chromy commented Oct 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

linear bot commented Oct 29, 2025

Uh oh!

This comment was marked as outdated.

Uh oh!

Uh oh!

This comment was marked as outdated.

Uh oh!

runningcode left a comment

Choose a reason for hiding this comment

Uh oh!

szokeasaurusrex left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szokeasaurusrex Oct 30, 2025

Choose a reason for hiding this comment

Uh oh!

chromy Oct 31, 2025

Choose a reason for hiding this comment

Uh oh!

szokeasaurusrex Oct 30, 2025

Choose a reason for hiding this comment

Uh oh!

chromy Oct 30, 2025

Choose a reason for hiding this comment

Uh oh!

chromy commented Oct 31, 2025

Uh oh!

chromy commented Oct 31, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

chromy commented Oct 29, 2025 •

edited

Loading

szokeasaurusrex left a comment •

edited

Loading